[BE] Upgrade runner lambdas from deprecated v2 client to v3 #5920
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
facebook-github-bot
added
the
CLA Signed
| @@ -41,7 +52,7 @@ export interface RunnerTypeOptional { | |||
|
|
|||
| export interface RunnerType extends RunnerTypeOptional { | |||
| disk_size: number; | |||
| instance_type: string; | |||
| instance_type: _InstanceType; | |||
There was a problem hiding this comment.
If it starts with a underscore _ it is not a public interface, and we should not be using it. Can't we cast this to a string or something else?
|
|
||
| const data = await expBackOff(() => { | ||
| return metrics.trackRequest( | ||
| metrics.smGetSecretValueAWSCallSuccess, | ||
| metrics.smGetSecretValueAWSCallFailure, | ||
| () => { | ||
| return secretsManager.getSecretValue({ SecretId: secretsManagerSecretsId }).promise(); | ||
| return secretsManager.getSecretValue({ SecretId: secretsManagerSecretsId }); |
There was a problem hiding this comment.
seems promise is still supported in the latest version.
https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Request.html#promise-property
Is there a reason you migrated from a async call to a sync call?
There was a problem hiding this comment.
The auto converter removed those. As per the migration guide, it seems like it automatically returns a promise without it needing to be explicitly referenced:
https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrating.html
| EncryptionContext: { | ||
| ['Environment']: environmentName, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
same here about the sync calling.
| @@ -220,7 +222,7 @@ export class Metrics { | |||
| `NS: ${metricsReq.Namespace}] (${i} of ${awsMetrics.length})`, | |||
| ); | |||
| await expBackOff(async () => { | |||
| return await this.cloudwatch.putMetricData(metricsReq).promise(); | |||
| return await this.cloudwatch.putMetricData(metricsReq); | |||
There was a problem hiding this comment.
please review all removals of the .promise() it might be a big problem
| const ec2Client = new EC2({ region: awsRegion }); | ||
| const command = new DescribeInstancesCommand({ Filters: ec2Filters }); | ||
| const describeInstanceResult = await ec2Client.send(command); | ||
| console.debug( |
| }); | ||
| }); | ||
| if (response.Failed == undefined) { |
There was a problem hiding this comment.
can't we update the if statements below over overwriting the response here?
|
I really like the initiative, but I believe we have some major things that we need to solve before moving forward with tests. What stands out the most is making sure we're properly returning Some functions are regular functions, that expect to return And, seems that there are some confusing changes on the |
v2 reached maintenance mode in Sept 2024 and will reach end of life Sept 2025
Context:
https://aws.amazon.com/blogs/developer/announcing-end-of-support-for-aws-sdk-for-javascript-v2/
Part of pytorch/pytorch#137228
Testing: ...none yet. Will try deploying these to canary and verify the autoscaling appears to work and logs show no obvious errors